refactor: upgrade alien_signals adapter#175
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR upgrades ChangesReactive system migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 939 892 -47
=========================================
- Hits 939 892 -47
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/solidart/test/solidart_test.dart (1)
2171-2178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert each helper’s effect separately.
runEffect()is followed byeffect.run(), so this test can still pass ifrunEffect()fails. Assert the count after each helper call, and assertpropagate/flushcauses the expected rerun.As per coding guidelines,
packages/solidart/test/**/*.dart: Write unit tests covering Signal operations and state transitions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solidart/test/solidart_test.dart` around lines 2171 - 2178, The test in solidart_test currently groups multiple helper calls before checking behavior, so a failure in runEffect() could be masked by effect.run(). Update the assertions around effect.subscriber, effect.run(), reactiveSystem.propagate(), and reactiveSystem.flush() so each helper’s effect is verified separately and the effect run count is checked after each step. Keep the existing symbols effectRuns, runEffect, effect.run, propagate, and flush in place, but split the expectations to confirm propagate/flush triggers the rerun as intended.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/solidart/lib/src/core/alien.dart`:
- Around line 29-31: The propagation check in Alien.currentValueUpdate is
comparing with oldValue != currentValue, which bypasses Computed’s custom
equality logic. Update the currentValue mutation path in alien.dart to use the
same comparison behavior as Computed/_compare (or route through the Computed
comparator/equality helper) so change propagation respects custom
equals/comparator semantics consistently.
- Around line 78-80: The comparison in alien.dart is collapsing None and
Some(null) by using safeUnwrap() on both values, which breaks nullable signal
transitions. Update the comparison logic in the affected _compare call so it
preserves the distinction between an uninitialized value and an explicit null
value, and ensure the hasValue/reactivity path in the surrounding signal
handling uses that distinction when deciding whether to notify observers.
---
Nitpick comments:
In `@packages/solidart/test/solidart_test.dart`:
- Around line 2171-2178: The test in solidart_test currently groups multiple
helper calls before checking behavior, so a failure in runEffect() could be
masked by effect.run(). Update the assertions around effect.subscriber,
effect.run(), reactiveSystem.propagate(), and reactiveSystem.flush() so each
helper’s effect is verified separately and the effect run count is checked after
each step. Keep the existing symbols effectRuns, runEffect, effect.run,
propagate, and flush in place, but split the expectations to confirm
propagate/flush triggers the rerun as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54b0e40b-846b-4d28-90b3-9d858422482c
📒 Files selected for processing (10)
packages/flutter_solidart/lib/src/widgets/signal_builder.dartpackages/flutter_solidart/test/flutter_solidart_test.dartpackages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/core.dartpackages/solidart/lib/src/core/effect.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/pubspec.yamlpackages/solidart/test/solidart_test.dart
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/solidart/analysis_options.yaml (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid package-wide suppression for
unreachable_from_mainunless strictly necessary.Line 7 mutes this diagnostic across the whole package; prefer a targeted file-level ignore where the warning is expected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solidart/analysis_options.yaml` at line 7, The package-wide ignore for unreachable_from_main in analysis_options.yaml should be removed or narrowed to the specific file(s) that actually need it. Update the analysis_options configuration to avoid suppressing the diagnostic for the entire package, and apply a targeted file-level ignore only where the warning is expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/solidart/analysis_options.yaml`:
- Line 7: The package-wide ignore for unreachable_from_main in
analysis_options.yaml should be removed or narrowed to the specific file(s) that
actually need it. Update the analysis_options configuration to avoid suppressing
the diagnostic for the entire package, and apply a targeted file-level ignore
only where the warning is expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c97703c6-f715-4cb0-95ce-fc669879bd6c
📒 Files selected for processing (3)
packages/flutter_solidart/test/flutter_solidart_test.dartpackages/solidart/analysis_options.yamlpackages/solidart/test/solidart_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solidart/test/solidart_test.dart
- Remove update() wrapper method; call didUpdate() directly on signal/computed nodes - Replace activeSub setter with explicit setCurrentSub() for save/restore semantics - Use named ReactiveFlags constants instead of magic bits for clarity - Add detailed comments on reactive system edge cases and unsafe internals
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/solidart/test/solidart_test.dart (1)
2142-2149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the disposal-order check observable during recomputation.
This verifies the child effect is gone after
computed.valueis read, but it would still pass if cleanup happened after the selector recomputed. Add an observable action in the false branch so the test fails unless the old child effect is disposed before recomputation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solidart/test/solidart_test.dart` around lines 2142 - 2149, The disposal-order assertion in the Computed test is not observable during recomputation because the false branch of the selector does nothing. Update the Computed callback to include an observable action in the else/false path so the test can detect whether the child Effect created inside the source.value branch has been disposed before the selector recomputes. Keep the change localized to the Computed and Effect setup in solidart_test.dart, using the existing childRuns/source.value structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/solidart/test/solidart_test.dart`:
- Around line 2193-2213: The test is conflating normal signal propagation with
`reactiveSystem.propagate()`, so the helper is not being isolated. In
`solidart_test.dart`, update the `reactiveSystem.propagate(subs!)` scenario in
the batching block so it runs without a preceding `signal.value++`, or add a
state assertion that only `propagate()` can trigger; keep `flush()` validating
that specific path. While touching this area, consider asserting the return
value from `reactiveSystem.setCurrentSub()` near the existing `activeSub` checks
to cover the save/restore behavior directly.
---
Outside diff comments:
In `@packages/solidart/test/solidart_test.dart`:
- Around line 2142-2149: The disposal-order assertion in the Computed test is
not observable during recomputation because the false branch of the selector
does nothing. Update the Computed callback to include an observable action in
the else/false path so the test can detect whether the child Effect created
inside the source.value branch has been disposed before the selector recomputes.
Keep the change localized to the Computed and Effect setup in
solidart_test.dart, using the existing childRuns/source.value structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab90cf8b-484c-4ace-a980-20e22480f75d
📒 Files selected for processing (5)
packages/solidart/lib/src/core/alien.dartpackages/solidart/lib/src/core/computed.dartpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/test/solidart_test.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/solidart/lib/src/core/read_signal.dart
- packages/solidart/lib/src/core/alien.dart
- packages/solidart/lib/src/core/reactive_system.dart
Assert setCurrentSub's save/restore return value, and pin each batch-helper's contribution (startBatch depth, batched no-flush, propagate idempotency, flush as the trigger, endBatch depth). Addresses CodeRabbit review feedback on PR #175.
ReadableSignal.dispose now unlinks each subscriber via alien.unlink (both sides of the dependency link) instead of a hand-rolled one-sided null-out. This removes a dangling subs link that let a write to a disposed signal propagate into an already-detached computed, leaving it pending with null deps. With the graph kept consistent, the defensive guard in getComputedValue is no longer needed and is removed. Adds a regression test for the post-dispose write, and strengthens the computed child-effect test to assert no stale children accumulate across recomputes.
solidart 2.9.0, flutter_solidart 2.7.5 (requires solidart ^2.9.0). Documents the alien_signals ^2.3.1 adapter upgrade, the lazy-nullable-Signal-null fix, and the signal-dispose unlink fix.
Debug-only sentinel: a computed must never be pending while detached (deps == null). Replaces the removed runtime guard with a documented invariant check that fails loudly in tests if a half-linked node ever reappears (e.g. a one-sided unlink), instead of crashing cryptically in upstream ComputedNode.get(). Stripped in release builds.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/solidart/lib/src/core/read_signal.dart`:
- Around line 277-290: The disposal path in read_signal.dart is still gated by
SolidartConfig.autoDispose, which leaves subscribers attached after manual
dispose() when the global default is off. Move the unlink loop in the signal
disposal logic so it always runs for disposed signals, using alien.unlink and
the subsequent _mayDispose() calls on _AlienEffect and _AlienComputed to let
each subscriber decide its own disposal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b24e4b3-90d5-4a9c-a9ce-ed863ed93287
📒 Files selected for processing (7)
packages/flutter_solidart/CHANGELOG.mdpackages/flutter_solidart/pubspec.yamlpackages/solidart/CHANGELOG.mdpackages/solidart/lib/src/core/reactive_system.dartpackages/solidart/lib/src/core/read_signal.dartpackages/solidart/pubspec.yamlpackages/solidart/test/solidart_test.dart
💤 Files with no reviewable changes (1)
- packages/solidart/lib/src/core/reactive_system.dart
✅ Files skipped from review due to trivial changes (2)
- packages/flutter_solidart/CHANGELOG.md
- packages/solidart/CHANGELOG.md
Regression test for SolidartConfig.autoDispose = false: disposing a source signal (which skips the unlink path) then writing to it must keep the getComputedValue pending/deps invariant and not crash. Guards the universal validity of the debug sentinel.
A Computed with multiple source signals was prematurely auto-disposed when one source was disposed: the old one-sided unlink in ReadableSignal.dispose corrupted the computed's dependency list so _mayDispose saw no deps. Now fixed by the two-sided alien.unlink; this test pins it down (computed survives one source being disposed, keeps reacting to the other, and auto-disposes only once all sources are gone). Reproduces the issue apps worked around with autoDispose: false.
nank1ro
force-pushed
the
refactor/upgrade-alien-signals
branch
from
53611f3 to
cd06344
Compare
Computed.listenerCount returned _deps.length (the number of dependencies) instead of the number of subscribers, unlike ReadableSignal.listenerCount. This also fed a wrong check in Resource.dispose (`source.listenerCount == 0`), which decides whether to dispose a Computed passed as a Resource's `source` — so a Computed source could be skipped by that cleanup. RED test added (a 2-dependency computed reported 2 listeners with 0 observers).
Computed.dispose() went through alien.stop(), which unlinks the node's deps but only the FIRST subscriber link, leaving any further subscribers holding a dangling link to a disposed computed (observable as listenerCount > 0 after dispose with 2+ subscribers). Now unlinks all subscribers, mirroring the signal-dispose fix. Also: extract a shared subscriberCount helper and drop the redundant _subs Set (its only remaining use was listenerCount; it was rebuilt on every hot read), and add a guard test asserting alien_signals' hidden hasChildEffect flag is still 64.
…oDispose ReadableSignal.dispose gated its subscriber-unlink behind the global SolidartConfig.autoDispose, so with auto-dispose off a disposed signal kept all its links and still propagated writes to subscribers — inconsistent with Effect.dispose and Computed.dispose, which always tear down. dispose() now always unlinks (the per-subscriber _mayDispose still self-guards on each subscriber's own autoDispose flag).
ReactiveSystem, reactiveSystem and the MayDisposeDependencies extension expose internal alien_signals types (ReactiveNode/Link) in their signatures, yet were exported from the public solidart.dart barrel. Hide them from the public barrel and expose them to sibling packages via a dedicated package:solidart/solidart_internal.dart. flutter_solidart's SignalBuilder now imports reactiveSystem from there. No public behaviour change for application code.
Disposing a Computed unlinked its subscribers but, unlike ReadableSignal.dispose, never offered them _mayDispose — so an autoDispose effect that only observed a disposed computed leaked. Extract the shared unlinkSubscribers helper (unlink each subscriber + offer _mayDispose) and use it from both ReadableSignal.dispose and _AlienComputed.dispose, removing the duplicated loop. RED-first regression test added.
Bump solidart and flutter_solidart to 3.0.0-dev.1 and solidart_hooks to 4.0.0-dev.1 (requiring flutter_solidart ^3.0.0-dev.1). Update workspace example constraints to ^3.0.0-dev.1 so they build against the local prerelease, and add path dependency_overrides to the non-workspace solidart_lint example so it uses the latest local packages too.
…t across types BREAKING: SolidartConfig.autoDispose now defaults to false — auto-dispose is opt-in from v3. Also make Effect honour its per-instance autoDispose flag (defaulted from SolidartConfig.autoDispose at creation) like Signal and Computed; previously Effect._mayDispose re-checked the global flag, so a per-instance autoDispose: true was ignored when the global was off. Auto-dispose widget tests now run with the default both ON and OFF, asserting entities dispose iff auto-dispose is enabled.
solidart_lint now depends on solidart/flutter_solidart ^3.0.0-dev.1; bump to its own dev prerelease so the workspace is on a coherent 3.0.0-dev.1 release.
Bumping to the 3.0 major triggers this lint for Resource's deprecated value/previousState/untrackedValue/on/maybeOn aliases. They are load-bearing (route through `state`, which triggers the resolution that `until`/`untilReady` rely on), so they can't be dropped yet — ignore the lint for now.
With the new opt-in auto-dispose default, no existing test exercised the source-disposal branch of Resource.dispose (an autoDispose source with no remaining listeners). Add one, restoring 100% coverage.
…line Replace the project-wide lint suppression with per-line // ignore comments on Resource's load-bearing deprecated aliases (value/previousState/untrackedValue/on/maybeOn), which can't be removed for 3.0 yet.
Rename the example package (example -> solidart_lint_example) so it no longer collides with flutter_solidart/example, add it to the workspace, and drop the path dependency_overrides. Bump its flutter_lints to ^6.0.0 to match the workspace. No dependency_overrides remain in the repo.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #156
closes #162
Summary
Supersedes #167 with a narrower branch that only upgrades Solidart's alien_signals integration.
alien_signalsto^2.3.1.SignalBuilderthrough the reactive helper instead of directactiveSubassignment.Validation
flutter pub getflutter analyze packagesflutter test packages --coverage -r github --fail-fastCoverage stays at
100.0000%locally after the upgrade (894/894).Summary by CodeRabbit
nullcorrectly notifies dependent effects and computed values.